feat(desktop): add design reference to html skill#376
Conversation
There was a problem hiding this comment.
Review mode: initial
Findings
-
[Minor] The skill file references
craft-polishin itsdependencieslist, but thecraft-polishskill was not found in the public skills directory underapps/desktop/resources/templates/skills/. The loader test does not check for it, and the existing skill set (from the test) does not includecraft-polish. This may cause confusion if the agent or loader attempts to resolve the dependency at runtime. Either add the missingcraft-polishskill, remove the dependency, or document that skill dependencies are advisory only.
Suggested fix: Addcraft-polishas a companion skill, or changedependenciesto an empty list if it isn't required. -
[Nit] The CSS example in the skill uses
var(--reference-ratio)andvar(--page-bg)without defining or referencing where those CSS custom properties come from. Consider adding a brief note that the agent should define these variables in the workspace stylesheet orDESIGN.md.
Summary
This PR adds a new built-in design-reference-to-html skill for recreating UI screenshots/mockups as HTML or App.jsx. The skill is well-structured, follows the project's architectural conventions (workspace files, preview(), gen_image(), etc.), and avoids vendoring external resources. A changeset is included, and the existing skill loader test is updated to verify the new skill is loaded. No security, license, or data-loss concerns. The craft-polish dependency is a minor gap that should be addressed before or shortly after merging.
Testing
- Added one assertion to verify the skill is loaded by
loadSkillsFromDir()(line 72 ofloader.test.ts). This is sufficient for a new skill file. - Not checked: a test that the skill content parses correctly and that
validationHints/dependenciesare handled properly by the loader.
Open-CoDesign Bot
2b82eb9 to
619a413
Compare
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
-
[Major] The test in
packages/core/src/skills/loader.test.tsnow asserts thatcraft-polishis loaded (line 72), but this PR did not add acraft-polish.mdskill file toapps/desktop/resources/templates/skills/. The test will fail because the skill is not present in the builtin directory. Either add the missingcraft-polish.mdskill or remove the assertion from the test. The previous minor finding is not resolved and is now escalated.
Suggested fix: Create acraft-polish.mdskill with the expected frontmatter, or removeexpect(ids).toContain('craft-polish');from the test if the dependency is only advisory. -
[Minor] The
dependenciesfield in the skill frontmatter still includescraft-polish. If the dependency is not actually required or not yet implemented, remove it or add a comment that dependencies are advisory only. The test change does not fix the missing skill.
Suggested fix: Removecraft-polishfrom thedependencieslist in the skill frontmatter if it is not a real dependency, or add the skill file.
Resolved from previous review
- The CSS custom properties note (
--page-bg,--reference-ratio) is addressed: the skill body now says "Define--page-bgand--reference-ratioin the artifact stylesheet".
Summary
The PR adds a well-structured design-reference-to-html skill with a changeset and test coverage. However, the test now expects a craft-polish skill that does not exist in this PR's file set, causing a test failure. This must be fixed before merging. No security, license, or data-loss concerns.
Testing
- The loader test now includes a passing assertion for
design-reference-to-htmlbut a failing assertion forcraft-polish. The test will not pass as-is. - No additional skill content tests were added.
Open-CoDesign Bot
Signed-off-by: Sun-sunshine06 <Sun-sunshine06@users.noreply.github.com>
619a413 to
cc9b1fe
Compare
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
No new issues found. All prior findings have been resolved.
- [Major (resolved)] The previous test assertion for
craft-polishthat would have caused a test failure has been removed from the diff. The current test only adds the correctdesign-reference-to-htmlassertion. - [Minor (resolved)] The skill
dependenciesfield now correctly lists only[responsive-layout]. Thecraft-polishdependency has been removed.
Summary
This PR adds a well-structured design-reference-to-html method skill, a corresponding changeset (@open-codesign/desktop minor), and a minimal test update. The skill is self-contained, uses only permissive text content, and follows the project's skill conventions (frontmatter, aliases, validation hints). No security, license, data-loss, or performance concerns. The PR is ready to merge.
Residual observations
- None.
Testing
- The existing loader test now asserts that
design-reference-to-htmlis loaded among builtin skills. - No additional skill-specific tests were added (e.g., testing that the skill is parseable and validates). This is acceptable given the existing test coverage for skill loading and the small scope of this change.
Open-CoDesign Bot
Summary
design-reference-to-htmlmethod skill for recreating screenshots/mockups asApp.jsx, HTML, or CSSDESIGN.md,preview(), and optionalgen_image()Testing
pnpm lintpnpm --filter @open-codesign/core test -- src/skills/loader.test.ts src/resource-manifest.test.ts src/tools/skill.test.tspnpm --filter @open-codesign/core test -- src/skills/loader.test.tspnpm --filter @open-codesign/ui test@open-codesign/ui, then@open-codesign/uipassed standalone